connectivity: extend node to node encryption tests #1870

3u13r · 2023-07-27T09:45:51Z

This PR extends the node-to-node encryption tests. Before no actions would fail when running them with encryption disabled (Before cilium/cilium#26712 it was 1/3, because the tunneling feature wasn't correctly detected) in one of my environments (WireGuard, tunnel, vxlan, endpointRoutes). I've adapted them so they work with native routing and tunneling with WireGuard (as far as I could test it), i.e. all actions fail when encryption is disabled and succeed if it is enabled.

The first two commits are just some minor cleanup and documentation in the feature detection. Feedback regarding the detection preference comment is especially welcome.

Output before:

[=] Test [node-to-node-encryption]
  [-] Scenario [node-to-node-encryption/node-to-node-encryption]
  🐛 Detected cilium_vxlan iface for communication among client and server nodes
  🐛 Running in bg: tcpdump -i cilium_vxlan --immediate-mode -w /tmp/node-to-node-encryption.pcap src host 10.244.2.111 and dst host 192.168.179.114 and icmp -c 1
  [.] Action [node-to-node-encryption/node-to-node-encryption/ping-ipv4: cilium-test/client-c4bfddc44-jgzxz (10.244.2.111) -> cilium-test/host-netns-h6nn8 (192.168.179.114:0)]
  🐛 Executing command [ping -c 1 -W 2 -w 10 192.168.179.114]
  🐛 Detected cilium_vxlan iface for communication among client and server nodes
  🐛 Running in bg: tcpdump -i cilium_vxlan --immediate-mode -w /tmp/node-to-node-encryption.pcap src host 192.168.179.108 and dst host 192.168.179.114 and icmp -c 1
  [.] Action [node-to-node-encryption/node-to-node-encryption/ping-ipv4: cilium-test/host-netns-2k57p (192.168.179.108) -> cilium-test/host-netns-h6nn8 (192.168.179.114:0)]
  🐛 Executing command [ping -c 1 -W 2 -w 10 192.168.179.114]
  🐛 Detected cilium_vxlan iface for communication among client and server nodes
  🐛 Running in bg: tcpdump -i cilium_vxlan --immediate-mode -w /tmp/node-to-node-encryption.pcap src host 192.168.179.108 and dst host 10.244.1.159 and tcp -c 1
  [.] Action [node-to-node-encryption/node-to-node-encryption/curl-ipv4: cilium-test/host-netns-2k57p (192.168.179.108) -> cilium-test/echo-other-node-646976b7dd-nxl4x (10.244.1.159:8080)]
  🐛 Executing command [curl -w %{local_ip}:%{local_port} -> %{remote_ip}:%{remote_port} = %{response_code} --silent --fail --show-error --output /dev/null --connect-timeout 2 --max-time 10 http://10.244.1.159:8080]

Output after:

[=] Test [node-to-node-encryption]
  [-] Scenario [node-to-node-encryption/node-to-node-encryption]
  🐛 Running /bin/sh -c ip -o route get 192.168.179.114 | grep -oE 'dev [^ ]*' | cut -d' ' -f2
  🐛 Running /bin/sh -c ip -o route get 192.168.179.114 | grep -oE 'src [^ ]*' | cut -d' ' -f2
  🐛 Running in bg: tcpdump -i ens5 --immediate-mode -w /tmp/node-to-node-encryption.pcap src host 192.168.179.108 and dst host 192.168.179.114 and icmp
  [.] Action [node-to-node-encryption/node-to-node-encryption/ping-ipv4: cilium-test/client-c4bfddc44-jgzxz (10.244.2.111) -> cilium-test/host-netns-h6nn8 (192.168.179.114:0)]
  🐛 Executing command [ping -c 1 -W 2 -w 10 192.168.179.114]
  ❌ Captured unencrypted pkt (count=1 packet)
  🐛 Captured pkts:
10:44:05.285615 IP 192.168.179.108 > 192.168.179.114: ICMP echo request, id 11628, seq 1, length 64

  🐛 Running /bin/sh -c ip -o route get 192.168.179.114 | grep -oE 'dev [^ ]*' | cut -d' ' -f2
  🐛 Running /bin/sh -c ip -o route get 192.168.179.114 | grep -oE 'src [^ ]*' | cut -d' ' -f2
  🐛 Running in bg: tcpdump -i ens5 --immediate-mode -w /tmp/node-to-node-encryption.pcap src host 192.168.179.108 and dst host 192.168.179.114 and icmp
  [.] Action [node-to-node-encryption/node-to-node-encryption/ping-ipv4: cilium-test/host-netns-2k57p (192.168.179.108) -> cilium-test/host-netns-h6nn8 (192.168.179.114:0)]
  🐛 Executing command [ping -c 1 -W 2 -w 10 192.168.179.114]
  ❌ Captured unencrypted pkt (count=1 packet)
  🐛 Captured pkts:
10:44:07.285568 IP 192.168.179.108 > 192.168.179.114: ICMP echo request, id 2, seq 1, length 64

  🐛 Running /bin/sh -c ip -o route get 10.244.1.159 | grep -oE 'dev [^ ]*' | cut -d' ' -f2
  🐛 Running /bin/sh -c ip -o route get 10.244.1.159 | grep -oE 'src [^ ]*' | cut -d' ' -f2
  🐛 Running in bg: tcpdump -i cilium_vxlan --immediate-mode -w /tmp/node-to-node-encryption.pcap src host 10.244.2.4 and dst host 10.244.1.159 and tcp
  [.] Action [node-to-node-encryption/node-to-node-encryption/curl-ipv4: cilium-test/host-netns-2k57p (192.168.179.108) -> cilium-test/echo-other-node-646976b7dd-nxl4x (10.244.1.159:8080)]
  🐛 Executing command [curl -w %{local_ip}:%{local_port} -> %{remote_ip}:%{remote_port} = %{response_code} --silent --fail --show-error --output /dev/null --connect-timeout 2 --max-time 10 http://10.244.1.159:8080]
  ❌ Captured unencrypted pkt (count=6 packets)
  🐛 Captured pkts:
10:44:09.107766 IP 10.244.2.4.55498 > 10.244.1.159.8080: Flags [S], seq 1124524169, win 64860, options [mss 1410,sackOK,TS val 2752214173 ecr 0,nop,wscale 7], length 0
10:44:09.108262 IP 10.244.2.4.55498 > 10.244.1.159.8080: Flags [.], ack 766383770, win 507, options [nop,nop,TS val 2752214174 ecr 3559090258], length 0
10:44:09.108331 IP 10.244.2.4.55498 > 10.244.1.159.8080: Flags [P.], seq 0:80, ack 1, win 507, options [nop,nop,TS val 2752214174 ecr 3559090258], length 80: HTTP: GET / HTTP/1.1
10:44:09.109436 IP 10.244.2.4.55498 > 10.244.1.159.8080: Flags [.], ack 2446, win 498, options [nop,nop,TS val 2752214175 ecr 3559090259], length 0
10:44:09.109563 IP 10.244.2.4.55498 > 10.244.1.159.8080: Flags [F.], seq 80, ack 2446, win 502, options [nop,nop,TS val 2752214175 ecr 3559090259], length 0
10:44:09.110085 IP 10.244.2.4.55498 > 10.244.1.159.8080: Flags [.], ack 2447, win 502, options [nop,nop,TS val 2752214176 ecr 3559090260], length 0

maintainer-s-little-helper · 2023-07-27T09:45:54Z

Commits 7c5c9d4, 05a5030, 7c7653d, 3e0b341 do not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

3u13r · 2023-08-01T11:19:28Z

I'm already working on the follow-up PR to this (porting the pod-to-pod-strict-mode test to the cilium-cli). Since the future changes are based on this PR, could someone maybe take a look. Maybe @brb?

maintainer-s-little-helper · 2023-08-02T10:46:16Z

Commit e9833c2 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

maintainer-s-little-helper · 2023-08-02T10:46:59Z

Commit e9833c2 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

3u13r · 2023-08-04T09:05:03Z

I rebased the PR to main to fix the CI. btw: the follow-up code implementing the strict mode pod-to-pod test is already finished. I'll create a PR once this is merged, as I depend on this PR.

3u13r · 2023-08-10T14:31:20Z

Are there any updates regarding the timeline of the review?

brb · 2023-08-11T15:36:43Z

@3u13r Sorry, a bit swamped. I will try to review in 1-2 weeks.

3u13r · 2023-08-14T08:29:15Z

No worries, just want to make sure that this PR isn't forgotten.

brb

Thanks. I did a quick skim of the PR. I will do the review once the commit msgs have been extended.

connectivity/check/features.go

connectivity/tests/encryption.go

The default value is not necessarily reflected in the ConfigMap. Therefore, we need to set the right values when the keys are not found. Accoding to the documented defaults in Cilium's deployment, tunneling with vxlan is enabled if not specified otherwise. Signed-off-by: Leonard Cohnen <[email protected]>

Signed-off-by: Leonard Cohnen <[email protected]>

When tunneling is enabled, and the feature is correctly picked up the the cilium-cli, the encryption test always listens on the tunneling interface for leaked packets. This is wrong as e.g. node-to-node communication is not encapsulated and therefore does not pass the cilium_vxlan interface. Signed-off-by: Leonard Cohnen <[email protected]>

Signed-off-by: Leonard Cohnen <[email protected]>

3u13r · 2023-08-14T13:19:46Z

I extended both commit messages. I assume that the other two commits are small enough to be explained by only the commit message. If not, I'm happy to extend those also.
It seems to me that the failing test is unrelated to the code I changed.

brb · 2023-08-16T09:31:24Z

Thanks! HOWTO test it against Cilium (we should put it into the contrib docs):

Get the CLI CI image tag from https://github.com/cilium/cilium-cli/actions/runs/5855144718/job/15872443885?pr=1870.
Open a draft PR against cilium/cilium with the image tag set in https://github.com/cilium/cilium/blob/main/.github/workflows/conformance-e2e.yaml#L56 and https://github.com/cilium/cilium/blob/main/.github/workflows/conformance-e2e.yaml#L55 unset.
Comment /ci-e2e in that PR.

3u13r · 2023-08-16T14:56:32Z

Done. Not 100% how to interpret the test failure but I assume that it is unrelated. On the other hand, I couldn't find a recent similar failure.

brb · 2023-08-17T08:43:09Z

Thanks. The ci-e2e has passed (https://github.com/cilium/cilium/actions/runs/5880176520). The rest is irrelevant.

3u13r · 2023-08-17T09:49:53Z

ci-ipsec-e2e also looks good: https://github.com/cilium/cilium/actions/runs/5889185248

brb

Thanks!

Unrelated to your PR, but the encryption leak test is a bit brittle, as it depends on the src and netdev derivations, which might become wrong, thus giving a false negative.

To eliminate the src detection, we could use a unique server port, and use it in the tcpdump filter.

However, improving/eliminating the netdev detection is not that trivial.

3u13r · 2023-08-18T12:20:01Z

Thanks for the review and the pointers for testing it against cilium!

I can have a look at making this test less brittle. What I always do locally is to disable the encryption and check that the test fails. Since the tests are run in the ci, inverting the test when encryption is disabled is technically an option. But I'll look at the simpler options first.

3u13r requested a review from a team as a code owner July 27, 2023 09:45

3u13r requested a review from derailed July 27, 2023 09:45

maintainer-s-little-helper bot added the dont-merge/needs-sign-off label Jul 27, 2023

3u13r temporarily deployed to ci July 27, 2023 09:45 — with GitHub Actions Inactive

3u13r force-pushed the 3u13r/feat/node-to-node-encryption-tests branch from 3e0b341 to 58b7b18 Compare July 27, 2023 09:54

maintainer-s-little-helper bot removed the dont-merge/needs-sign-off label Jul 27, 2023

3u13r temporarily deployed to ci July 27, 2023 09:54 — with GitHub Actions Inactive

brb self-requested a review August 1, 2023 11:20

maintainer-s-little-helper bot added the dont-merge/needs-sign-off label Aug 2, 2023

3u13r had a problem deploying to ci August 2, 2023 10:46 — with GitHub Actions Error

3u13r force-pushed the 3u13r/feat/node-to-node-encryption-tests branch from e9833c2 to b4bfd7b Compare August 2, 2023 10:46

3u13r temporarily deployed to ci August 2, 2023 10:47 — with GitHub Actions Inactive

3u13r force-pushed the 3u13r/feat/node-to-node-encryption-tests branch from b4bfd7b to 7ed9b37 Compare August 2, 2023 11:12

maintainer-s-little-helper bot removed the dont-merge/needs-sign-off label Aug 2, 2023

3u13r temporarily deployed to ci August 2, 2023 11:12 — with GitHub Actions Inactive

brb reviewed Aug 14, 2023

View reviewed changes

connectivity/check/features.go Show resolved Hide resolved

connectivity/tests/encryption.go Show resolved Hide resolved

3u13r added 4 commits August 14, 2023 11:55

connectivity: document feature detection order

7c54c5f

Signed-off-by: Leonard Cohnen <[email protected]>

connectivity: print all captured packets

9e40129

Signed-off-by: Leonard Cohnen <[email protected]>

3u13r force-pushed the 3u13r/feat/node-to-node-encryption-tests branch from 7ed9b37 to 9e40129 Compare August 14, 2023 11:27

3u13r temporarily deployed to ci August 14, 2023 11:27 — with GitHub Actions Inactive

3u13r requested a review from brb August 15, 2023 08:44

3u13r mentioned this pull request Aug 16, 2023

WIP: test cilium-cli pr 1870 cilium/cilium#27539

Closed

brb approved these changes Aug 18, 2023

View reviewed changes

brb added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. area/CI Continuous Integration testing issue or flake labels Aug 18, 2023

michi-covalent merged commit 33ddb44 into cilium:main Aug 21, 2023
19 checks passed

p-linnane mentioned this pull request Aug 31, 2023

cilium-cli 0.15.7 Homebrew/homebrew-core#141024

Merged

3u13r deleted the 3u13r/feat/node-to-node-encryption-tests branch September 18, 2023 16:57

giorio94 mentioned this pull request Oct 17, 2023

Make encryption-related connectivity tests more reliable #2035

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

connectivity: extend node to node encryption tests #1870

connectivity: extend node to node encryption tests #1870

3u13r commented Jul 27, 2023 •

edited

Loading

maintainer-s-little-helper bot commented Jul 27, 2023

3u13r commented Aug 1, 2023

maintainer-s-little-helper bot commented Aug 2, 2023

maintainer-s-little-helper bot commented Aug 2, 2023

3u13r commented Aug 4, 2023 •

edited

Loading

3u13r commented Aug 10, 2023

brb commented Aug 11, 2023

3u13r commented Aug 14, 2023

brb left a comment

3u13r commented Aug 14, 2023

brb commented Aug 16, 2023

3u13r commented Aug 16, 2023

brb commented Aug 17, 2023

3u13r commented Aug 17, 2023

brb left a comment

3u13r commented Aug 18, 2023

connectivity: extend node to node encryption tests #1870

connectivity: extend node to node encryption tests #1870

Conversation

3u13r commented Jul 27, 2023 • edited Loading

maintainer-s-little-helper bot commented Jul 27, 2023

3u13r commented Aug 1, 2023

maintainer-s-little-helper bot commented Aug 2, 2023

maintainer-s-little-helper bot commented Aug 2, 2023

3u13r commented Aug 4, 2023 • edited Loading

3u13r commented Aug 10, 2023

brb commented Aug 11, 2023

3u13r commented Aug 14, 2023

brb left a comment

Choose a reason for hiding this comment

3u13r commented Aug 14, 2023

brb commented Aug 16, 2023

3u13r commented Aug 16, 2023

brb commented Aug 17, 2023

3u13r commented Aug 17, 2023

brb left a comment

Choose a reason for hiding this comment

3u13r commented Aug 18, 2023

3u13r commented Jul 27, 2023 •

edited

Loading

3u13r commented Aug 4, 2023 •

edited

Loading